Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dependencies: update requirement pytest~=6.0 and use pyproject.toml #4410

Merged
merged 1 commit into from
Sep 29, 2020
Merged

Dependencies: update requirement pytest~=6.0 and use pyproject.toml #4410

merged 1 commit into from
Sep 29, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 27, 2020

Fixes #4340

Starting from v6.0, pytest supports using the pyproject.toml instead
of a pytest.ini to define its configuration. Given that this is
quickly becoming the Python packaging standard and allows us to reduce
the number of configuration files in the top level of the repository, we
increase the version requirement of pytest.

csadorf
csadorf reviewed Sep 28, 2020
View reviewed changes
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are some issues with this update since the CI tests are failing: https://github.com/aiidateam/aiida-core/pull/4410/checks?check_run_id=1172693876

Did you already have a look at this? Is this a pytest incompatibility?

@sphuber
Copy link
Contributor Author

sphuber commented Sep 28, 2020

It seems to be a problem with pylint-rerunfailures: pytest-dev/pytest-rerunfailures#128 . This is only a problem for pytest==6.1. We can either limit pytest<6.1, wait for a fix of pytest-rerunfailures or hold off with this PR.

@csadorf
Copy link
Contributor

csadorf commented Sep 28, 2020 •
edited
Loading

It seems to be a problem with pylint-rerunfailures: pytest-dev/pytest-rerunfailures#128 . This is only a problem for pytest==6.1. We can either limit pytest<6.1, wait for a fix of pytest-rerunfailures or hold off with this PR.

I believe that rerunfailures was not actually that successful in resolving some of the issues it was supposed to address. I would recommend to revert the related commit. @ltalirz Could you comment?

@ltalirz
Copy link
Member

ltalirz commented Sep 28, 2020

I believe that rerunfailures was not actually that successful in resolving some of the issues it was supposed to address.

I believe it resolves some issues but not all - some tests will continue to fail even if you run them N times in a row since the failure is related to some hidden state in the environment.

I don't have time now, but it would be interesting to gather some statistics from the travis logs on how often flaky tests are being successfully rerun by rerunfailures (it's reported in the output).

I would suggest to keep it and limit the pytest version instead until the mentioned issue is fixed (unless we really need pytest v6.1)

@sphuber sphuber force-pushed the fix/4340/support-pytest-6 branch from 8dcb5b5 to fbcfdb5 Compare September 29, 2020 12:15
@greschd
Copy link
Member

greschd commented Sep 29, 2020

FYI, pytest-rerunfailures has released 9.1.1 which supposedly fixes the problem: https://pypi.org/project/pytest-rerunfailures/#history

@sphuber
Copy link
Contributor Author

sphuber commented Sep 29, 2020

FYI, pytest-rerunfailures has released 9.1.1 which supposedly fixes the problem: https://pypi.org/project/pytest-rerunfailures/#history

Good call, I actually just updated the PR to put an upper limit. I even checked the issue but somehow didn't notice it had been closed 🤦‍♂️ Will revert the change

@sphuber sphuber force-pushed the fix/4340/support-pytest-6 branch 2 times, most recently from 2655260 to 1bc0e1c Compare September 29, 2020 12:44
@codecov
Copy link

codecov bot commented Sep 29, 2020 •
edited
Loading

Codecov Report

Merging #4410 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4410   +/-   ##
========================================
  Coverage    79.22%   79.22%           
========================================
  Files          475      475           
  Lines        34826    34826           
========================================
  Hits         27589    27589           
  Misses        7237     7237
Flag Coverage Δ
#django 73.07% <ø> (ø)
#sqlalchemy 72.30% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9144924...456115a. Read the comment docs.

@sphuber
Dependencies: update requirement pytest~=6.0 and use pyproject.toml
456115a 
Starting from v6.0, `pytest` supports using the `pyproject.toml` instead
of a `pytest.ini` to define its configuration. Given that this is
quickly becoming the Python packaging standard and allows us to reduce
the number of configuration files in the top level of the repository, we
increase the version requirement of `pytest`.

Note that we also require `pytest-rerunfailures>=9.1.1` because lower
versions are broken in combination with `pytest==6.1`. See the following:

   pytest-dev/pytest-rerunfailures#128

for details.
@sphuber sphuber force-pushed the fix/4340/support-pytest-6 branch from 1bc0e1c to 456115a Compare September 29, 2020 12:51
@sphuber sphuber requested a review from csadorf September 29, 2020 14:10
@sphuber
Copy link
Contributor Author

sphuber commented Sep 29, 2020

@csadorf this is ready for review. pytest-rerunfailures released 9.1.1 with a fix, which is now added as a minimum requirement

@sphuber sphuber merged commit af91a8b into aiidateam:develop Sep 29, 2020
@sphuber sphuber deleted the fix/4340/support-pytest-6 branch September 29, 2020 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csadorf csadorf csadorf approved these changes

@chrisjsewell chrisjsewell Awaiting requested review from chrisjsewell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support pytest 6
4 participants
@sphuber @csadorf @ltalirz @greschd